Optimize `unit_return_expecting_ord` by blyxyas · Pull Request #14905 · rust-lang/rust-clippy

blyxyas · 2025-05-27T23:31:22Z

This lint was previously written very clumsily, not short-circuiting and doing a lot of unnecessary work.

Now it makes sure to do the cheaper functions earlier and in general, is just smarter.
(I specifically focused on minimizing binder instantiation

Sadly, I'm not finding any relevant result in a benchmark. Still with the LLVM coverage instruments, the expensive bits are called lots of less times (The binder instantiation that I care about is reduced from 95k to 10k throughout our test suite).

changelog:[unit_return_expecting_ord]: Optimize the lint

rustbot · 2025-05-27T23:31:27Z

r? @samueltardieu

rustbot has assigned @samueltardieu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

samueltardieu

Nice, but you can use interned symbols instead of allocating strings, that can only help (if a little bit) with performances.

samueltardieu · 2025-05-28T10:30:34Z

+    fn_mut_trait: DefId,
+    ord_trait: Option<DefId>,
+    partial_ord_trait: Option<DefId>,
+) -> Vec<(usize, String)> {


Suggested change

) -> Vec<(usize, String)> {

) -> Vec<(usize, Symbol)> {

samueltardieu · 2025-05-28T10:30:51Z

                            .any(|ord| Some(ord.self_ty()) == return_ty_pred.term.as_type())
                        {
-                            args_to_check.push((i, "Ord".to_string()));
+                            args_to_check.push((i, String::from("Ord")));


Suggested change

args_to_check.push((i, String::from("Ord")));

args_to_check.push((i, sym::Ord));

samueltardieu · 2025-05-28T10:31:08Z

                            .any(|pord| pord.self_ty() == return_ty_pred.term.expect_type())
                        {
-                            args_to_check.push((i, "PartialOrd".to_string()));
+                            args_to_check.push((i, String::from("PartialOrd")));


Suggested change

args_to_check.push((i, String::from("PartialOrd")));

args_to_check.push((i, sym::PartialOrd));

I did it initially and reverted it with some mental gymnastics. I'll re-revert it.

samueltardieu

Except for the two nits (no need to call .as_str(), and you can inline the trait_name in format string), this LGTM. If you just fix those two places feel free to r=me.

samueltardieu · 2025-05-28T15:47:45Z

+                            format!(
+                                "this closure returns \
+                                   the unit type which also implements {}",
+                                trait_name.as_str()


Suggested change

format!(

"this closure returns \

the unit type which also implements {}",

trait_name.as_str()

format!(

"this closure returns \

the unit type which also implements {trait_name}"

samueltardieu · 2025-05-28T15:48:07Z

+                            format!(
+                                "this closure returns \
+                                   the unit type which also implements {}",
+                                trait_name.as_str()
+                            ),


Suggested change

format!(

"this closure returns \

the unit type which also implements {}",

trait_name.as_str()

),

format!(

"this closure returns \

the unit type which also implements {trait_name}"

),

This lint was previously written very clumsily, not shortcircuiting and doing a lot of unnecessary work. Now it makes sure to do the cheaper functions earlier and in general, just be smarter. (I specifically focused on minimizing binder instantiation) Also, avoid allocating unnecessarily

blyxyas · 2025-05-28T16:01:05Z

feel free to r=me

Sadly we don't have bors anymore, so PRs can't be merged in behalf of another person 😢

samueltardieu · 2025-05-28T16:05:24Z

I know, this is an inherited way of saying "feel free to press the merge button on my behalf as I take responsibility for it". But since I'm here I'll do it!

nnethercote · 2025-05-29T02:57:55Z

 declare_lint_pass!(UnitReturnExpectingOrd => [UNIT_RETURN_EXPECTING_ORD]);

-fn get_trait_predicates_for_trait_id<'tcx>(
+// For each


Is this comment unfinished?

nnethercote · 2025-05-29T03:04:02Z

-        {
-            preds.push(trait_pred);
+                .instantiate_bound_regions_with_erased(pred.kind().rebind(poly_trait_pred));
+            for (i, tid) in trait_ids.iter().enumerate() {


The slice/array approach is a bit hard to understand, you have to look at the callsite to understand it. I see you want to get the operations within the instantiate_bound_regions_with_erased call. I think you could do that by passing in three separate arguments and handling them in sequence here, instead of using a for loop? I think that would be easier to read.

rustbot assigned samueltardieu May 27, 2025

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label May 27, 2025

samueltardieu requested changes May 28, 2025

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels May 28, 2025

blyxyas force-pushed the minimize-interning branch from 7d0b957 to 2635ced Compare May 28, 2025 15:40

samueltardieu approved these changes May 28, 2025

View reviewed changes

blyxyas force-pushed the minimize-interning branch from 2635ced to 7e590de Compare May 28, 2025 16:00

samueltardieu added this pull request to the merge queue May 28, 2025

Merged via the queue into rust-lang:master with commit b90880d May 28, 2025
11 checks passed

nnethercote reviewed May 29, 2025

View reviewed changes

blyxyas added the G-performance-project Goal: For issues and PRs related to the Clippy Performance Project label Jun 27, 2025

	args_to_check.push((i, String::from("Ord")));
	args_to_check.push((i, sym::Ord));

	args_to_check.push((i, String::from("PartialOrd")));
	args_to_check.push((i, sym::PartialOrd));

Conversation

blyxyas commented May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented May 27, 2025

Uh oh!

samueltardieu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samueltardieu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blyxyas commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

samueltardieu commented May 28, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

blyxyas commented May 27, 2025 •

edited

Loading

blyxyas commented May 28, 2025 •

edited

Loading